Skip to content

feat(remote): add support for policy.json allow/deny#1013

Open
TerryHowe wants to merge 3 commits intooras-project:mainfrom
TerryHowe:feature-policy-json
Open

feat(remote): add support for policy.json allow/deny#1013
TerryHowe wants to merge 3 commits intooras-project:mainfrom
TerryHowe:feature-policy-json

Conversation

@TerryHowe
Copy link
Copy Markdown
Member

@TerryHowe TerryHowe commented Oct 6, 2025

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 6, 2025

Codecov Report

❌ Patch coverage is 85.80858% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.55%. Comparing base (0cbe218) to head (f548cdf).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
registry/remote/policy/policy.go 70.70% 25 Missing and 4 partials ⚠️
registry/remote/policy/requirement.go 92.00% 6 Missing and 4 partials ⚠️
registry/remote/policy/evaluator.go 96.42% 2 Missing ⚠️
registry/remote/repository.go 91.30% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1013      +/-   ##
==========================================
+ Coverage   82.31%   82.55%   +0.23%     
==========================================
  Files          67       70       +3     
  Lines        4949     5252     +303     
==========================================
+ Hits         4074     4336     +262     
- Misses        538      570      +32     
- Partials      337      346       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@TerryHowe TerryHowe force-pushed the feature-policy-json branch 2 times, most recently from 2c3dc9a to f782b54 Compare October 6, 2025 19:23
@Wwwsylvia Wwwsylvia requested a review from Copilot October 10, 2025 11:15
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds comprehensive support for the containers-policy.json format, enabling image access control policies based on allow/deny rules. The implementation includes policy management, evaluation, and integration with repository operations.

Key changes:

  • Implements policy evaluation with support for insecure accept, reject, and signature verification requirements (placeholders)
  • Adds policy integration to Repository struct with enforcement in Fetch, Push, and Resolve operations
  • Provides comprehensive test coverage including unit tests, integration tests, and edge case handling

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
registry/remote/repository.go Adds Policy field and checkPolicy method integration
registry/remote/repository_policy_test.go Tests policy enforcement in repository operations
registry/remote/policy/policy.go Core policy management with load/save/validation
registry/remote/policy/evaluator.go Policy evaluation engine for image access decisions
registry/remote/policy/requirement.go Policy requirement types and JSON marshaling
registry/remote/policy/policy_test.go Comprehensive policy functionality tests
registry/remote/policy/requirement_test.go Tests for requirement validation and types
registry/remote/policy/edge_cases_test.go Edge case and error condition testing
registry/remote/policy/example_test.go Example usage and documentation tests
docs/policy.md Documentation for the policy package

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@shizhMSFT shizhMSFT changed the title Feature add support for policy.json allow/deny feat: add support for policy.json allow/deny Oct 10, 2025
@shizhMSFT shizhMSFT changed the title feat: add support for policy.json allow/deny feat(remote): add support for policy.json allow/deny Oct 10, 2025
@TerryHowe TerryHowe force-pushed the feature-policy-json branch 3 times, most recently from f6bd8e2 to 39e9cd5 Compare October 28, 2025 18:09
@TerryHowe TerryHowe added the v3 Things belongs to version 3.x label Nov 17, 2025
@TerryHowe TerryHowe added this to the v3.0.0 milestone Nov 17, 2025
@TerryHowe TerryHowe force-pushed the feature-policy-json branch 2 times, most recently from 0860b2b to 719ca4b Compare November 23, 2025 19:36
@TerryHowe TerryHowe requested a review from sabre1041 as a code owner December 10, 2025 00:58
Copy link
Copy Markdown
Contributor

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No additional comments other than what @shizhMSFT shared

TerryHowe added a commit to TerryHowe/oras-go that referenced this pull request Feb 17, 2026
… allow/deny

Signed-off-by: Terry Howe <terrylhowe@gmail.com>
@TerryHowe
Copy link
Copy Markdown
Member Author

Resolved all of those comments with a slight tweak to the comment for IdentityMatchExact which I think is a bit better go style.

TerryHowe and others added 2 commits March 24, 2026 17:14
Signed-off-by: Terry Howe <terrylhowe@gmail.com>
Move policy code from internal/configuration to registry/remote/policy
as a public package so SDK users can create and manage policies. Add
platform-specific default path handling so GetDefaultPolicyPath only
falls back to /etc/containers/policy.json on Linux. Use interfaces for
signature verification instead of direct implementation.

Signed-off-by: Terry Howe <thowe@nvidia.com>
shizhMSFT

This comment was marked as duplicate.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +209 to +223
// GetRequirementsForImage returns the policy requirements for a given transport and scope.
// It follows the precedence rules: specific scope > transport default > global default.
func (p *Policy) GetRequirementsForImage(transport TransportName, scope string) PolicyRequirements {
// Check for transport-specific scope
if transportScopes, ok := p.Transports[transport]; ok {
// Try exact scope match first
if reqs, ok := transportScopes[scope]; ok {
return reqs
}

// Try transport default (empty scope)
if reqs, ok := transportScopes[""]; ok {
return reqs
}
}
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetRequirementsForImage() only checks an exact scope key and then falls back to the transport default/global default. The containers-policy.json format supports hierarchical scope matching (most-specific scope should win, e.g. docker.io/library applying to docker.io/library/nginx). With the current exact-match logic, common real-world policies will be ignored unless the caller provides an exact key. Consider implementing longest-prefix (most-specific) match for scopes before falling back to "" and the global default.

Copilot uses AI. Check for mistakes.
Comment on lines +357 to +399
path, err := GetDefaultPolicyPath()

homeDir, _ := os.UserHomeDir()
userPath := filepath.Join(homeDir, PolicyConfUserDir, PolicyConfFileName)

if _, statErr := os.Stat(userPath); statErr == nil {
// User policy file exists — should succeed with that path
if err != nil {
t.Fatalf("GetDefaultPolicyPath() error = %v", err)
}
if path != userPath {
t.Errorf("GetDefaultPolicyPath() = %v, want %v", path, userPath)
}
} else if systemPolicyPath != "" {
// No user policy but system path available (Linux)
if err != nil {
t.Fatalf("GetDefaultPolicyPath() error = %v", err)
}
if path != PolicyConfSystemPath {
t.Errorf("GetDefaultPolicyPath() = %v, want %v", path, PolicyConfSystemPath)
}
} else {
// No user policy and no system path (non-Linux) — should error
if err == nil {
t.Error("GetDefaultPolicyPath() should error on non-Linux without user policy")
}
}
}

// Test LoadDefault
func TestLoadDefault(t *testing.T) {
// Create a temporary policy file in the home directory
homeDir, err := os.UserHomeDir()
if err != nil {
t.Skip("Cannot get home directory")
}

userPolicyDir := filepath.Join(homeDir, PolicyConfUserDir)
userPolicyPath := filepath.Join(userPolicyDir, PolicyConfFileName)

// Clean up any existing test policy
defer os.Remove(userPolicyPath)

Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestGetDefaultPolicyPath branches based on whether the user’s real $HOME/.config/containers/policy.json currently exists, so its expectations depend on the machine/environment running the tests. This makes the test suite flaky and potentially environment-dependent. Prefer setting HOME to a temp dir and explicitly creating/removing the policy file to cover each branch deterministically.

Suggested change
path, err := GetDefaultPolicyPath()
homeDir, _ := os.UserHomeDir()
userPath := filepath.Join(homeDir, PolicyConfUserDir, PolicyConfFileName)
if _, statErr := os.Stat(userPath); statErr == nil {
// User policy file exists — should succeed with that path
if err != nil {
t.Fatalf("GetDefaultPolicyPath() error = %v", err)
}
if path != userPath {
t.Errorf("GetDefaultPolicyPath() = %v, want %v", path, userPath)
}
} else if systemPolicyPath != "" {
// No user policy but system path available (Linux)
if err != nil {
t.Fatalf("GetDefaultPolicyPath() error = %v", err)
}
if path != PolicyConfSystemPath {
t.Errorf("GetDefaultPolicyPath() = %v, want %v", path, PolicyConfSystemPath)
}
} else {
// No user policy and no system path (non-Linux) — should error
if err == nil {
t.Error("GetDefaultPolicyPath() should error on non-Linux without user policy")
}
}
}
// Test LoadDefault
func TestLoadDefault(t *testing.T) {
// Create a temporary policy file in the home directory
homeDir, err := os.UserHomeDir()
if err != nil {
t.Skip("Cannot get home directory")
}
userPolicyDir := filepath.Join(homeDir, PolicyConfUserDir)
userPolicyPath := filepath.Join(userPolicyDir, PolicyConfFileName)
// Clean up any existing test policy
defer os.Remove(userPolicyPath)
t.Run("user policy exists", func(t *testing.T) {
tempHome, err := os.MkdirTemp("", "policy-home-user-exists-*")
if err != nil {
t.Fatalf("failed to create temp home: %v", err)
}
defer os.RemoveAll(tempHome)
t.Setenv("HOME", tempHome)
userPolicyDir := filepath.Join(tempHome, PolicyConfUserDir)
userPolicyPath := filepath.Join(userPolicyDir, PolicyConfFileName)
if err := os.MkdirAll(userPolicyDir, 0755); err != nil {
t.Fatalf("failed to create user policy dir: %v", err)
}
f, err := os.Create(userPolicyPath)
if err != nil {
t.Fatalf("failed to create user policy file: %v", err)
}
f.Close()
path, err := GetDefaultPolicyPath()
if err != nil {
t.Fatalf("GetDefaultPolicyPath() error = %v", err)
}
if path != userPolicyPath {
t.Errorf("GetDefaultPolicyPath() = %v, want %v", path, userPolicyPath)
}
})
t.Run("user policy missing", func(t *testing.T) {
tempHome, err := os.MkdirTemp("", "policy-home-user-missing-*")
if err != nil {
t.Fatalf("failed to create temp home: %v", err)
}
defer os.RemoveAll(tempHome)
t.Setenv("HOME", tempHome)
path, err := GetDefaultPolicyPath()
if systemPolicyPath != "" {
// No user policy but system path available (Linux)
if err != nil {
t.Fatalf("GetDefaultPolicyPath() error = %v", err)
}
if path != PolicyConfSystemPath {
t.Errorf("GetDefaultPolicyPath() = %v, want %v", path, PolicyConfSystemPath)
}
} else {
// No user policy and no system path (non-Linux) — should error
if err == nil {
t.Error("GetDefaultPolicyPath() should error on non-Linux without user policy")
}
}
})
}
// Test LoadDefault
func TestLoadDefault(t *testing.T) {
// Use a temporary HOME directory to avoid depending on the real user environment.
tempHome, err := os.MkdirTemp("", "policy-home-loaddefault-*")
if err != nil {
t.Fatalf("failed to create temp home: %v", err)
}
defer os.RemoveAll(tempHome)
t.Setenv("HOME", tempHome)
userPolicyDir := filepath.Join(tempHome, PolicyConfUserDir)
userPolicyPath := filepath.Join(userPolicyDir, PolicyConfFileName)

Copilot uses AI. Check for mistakes.
Comment on lines +627 to +641
// Test with read-only directory
readOnlyDir := filepath.Join(tmpDir, "readonly")
if err := os.MkdirAll(readOnlyDir, 0555); err != nil {
t.Fatalf("failed to create read-only directory: %v", err)
}
defer os.Chmod(readOnlyDir, 0755) // Restore permissions for cleanup

policy := &Policy{
Default: PolicyRequirements{&Reject{}},
}

policyPath := filepath.Join(readOnlyDir, "policy.json")
err := policy.Save(policyPath)
if err == nil {
t.Error("Save() should fail for read-only directory")
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestPolicy_Save_ErrorCases relies on POSIX directory permissions (0555) to make the directory read-only. This can behave differently on Windows, and can also fail to produce an error when running with elevated privileges (e.g. root in CI), making the test non-portable/flaky. Consider skipping on platforms/users where permissions aren’t enforced, or use a deterministic failure case (e.g. invalid path / injecting a write failure).

Suggested change
// Test with read-only directory
readOnlyDir := filepath.Join(tmpDir, "readonly")
if err := os.MkdirAll(readOnlyDir, 0555); err != nil {
t.Fatalf("failed to create read-only directory: %v", err)
}
defer os.Chmod(readOnlyDir, 0755) // Restore permissions for cleanup
policy := &Policy{
Default: PolicyRequirements{&Reject{}},
}
policyPath := filepath.Join(readOnlyDir, "policy.json")
err := policy.Save(policyPath)
if err == nil {
t.Error("Save() should fail for read-only directory")
// Use a directory path where a file path is expected to force a deterministic failure.
invalidPath := filepath.Join(tmpDir, "as_directory")
if err := os.MkdirAll(invalidPath, 0755); err != nil {
t.Fatalf("failed to create directory for invalid path test: %v", err)
}
policy := &Policy{
Default: PolicyRequirements{&Reject{}},
}
// Passing a directory path should cause Save to fail on all platforms.
err := policy.Save(invalidPath)
if err == nil {
t.Error("Save() should fail when given a directory path instead of a file path")

Copilot uses AI. Check for mistakes.
Comment on lines +155 to +162
// Policy is an optional policy evaluator for allow/deny decisions.
// If nil, no policy enforcement is performed.
// Policies can be loaded from a file via [policy.Load] or constructed
// programmatically via [policy.NewPolicy]. The default file-based loading
// ([policy.LoadDefault]) uses platform-specific paths; see the policy
// package documentation for cross-platform details.
// Reference: https://man.archlinux.org/man/containers-policy.json.5.en
Policy *policy.Evaluator
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding an exported field to the public remote.Repository struct is a source-compatible breaking change for downstream users who construct Repository with positional composite literals (without field names). If preserving that compatibility matters, consider making policy configuration available via an option (e.g. RepositoryOptions) or an unexported field with setter methods, rather than adding a new exported struct field.

Suggested change
// Policy is an optional policy evaluator for allow/deny decisions.
// If nil, no policy enforcement is performed.
// Policies can be loaded from a file via [policy.Load] or constructed
// programmatically via [policy.NewPolicy]. The default file-based loading
// ([policy.LoadDefault]) uses platform-specific paths; see the policy
// package documentation for cross-platform details.
// Reference: https://man.archlinux.org/man/containers-policy.json.5.en
Policy *policy.Evaluator
// policy is an optional policy evaluator for allow/deny decisions.
// If nil, no policy enforcement is performed.
// Policies can be loaded from a file via [policy.Load] or constructed
// programmatically via [policy.NewPolicy]. The default file-based loading
// ([policy.LoadDefault]) uses platform-specific paths; see the policy
// package documentation for cross-platform details.
// Reference: https://man.archlinux.org/man/containers-policy.json.5.en
policy *policy.Evaluator

Copilot uses AI. Check for mistakes.
Comment on lines 319 to 333
// Fetch fetches the content identified by the descriptor.
func (r *Repository) Fetch(ctx context.Context, target ocispec.Descriptor) (io.ReadCloser, error) {
if err := r.checkPolicy(ctx, ""); err != nil {
return nil, err
}
return r.blobStore(target).Fetch(ctx, target)
}

// Push pushes the content, matching the expected descriptor.
func (r *Repository) Push(ctx context.Context, expected ocispec.Descriptor, content io.Reader) error {
if err := r.checkPolicy(ctx, ""); err != nil {
return err
}
return r.blobStore(expected).Push(ctx, expected, content)
}
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Policy enforcement is only applied in Fetch, Push, and Resolve, but other public entry points that perform registry operations (e.g. FetchReference, PushReference, Tag, Delete, Exists, Mount, Tags, etc.) still bypass checkPolicy(). This makes policy enforcement inconsistent and easy to accidentally circumvent from within the same API surface. Consider enforcing policy at a lower level (e.g. in the underlying blob/manifest stores or request creation) or ensuring all relevant repository operations call checkPolicy() with an appropriate fully-qualified reference.

Copilot uses AI. Check for mistakes.
Comment on lines +298 to +305
ref := r.Reference.String()
if reference != "" {
ref = reference
}

imageRef := policy.ImageReference{
Transport: policy.TransportNameDocker,
Scope: r.Reference.Repository,
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkPolicy() builds imageRef.Scope as r.Reference.Repository (e.g. library/nginx) and, for Resolve, may set imageRef.Reference to the raw tag/digest string (e.g. "latest"). containers-policy.json docker scopes/refs are typically matched against fully-qualified names (e.g. docker.io/library/nginx and docker.io/library/nginx:latest), so this will cause scope-specific policies to never match and can break signed-identity matching. Build scope as <registry>/<repository> and always pass a fully-qualified reference (e.g. by constructing a registry.Reference{Registry:..., Repository:..., Reference:...}.String() or reusing r.ParseReference).

Suggested change
ref := r.Reference.String()
if reference != "" {
ref = reference
}
imageRef := policy.ImageReference{
Transport: policy.TransportNameDocker,
Scope: r.Reference.Repository,
// Build a fully-qualified reference string for policy evaluation.
ref := r.Reference.String()
if reference != "" {
fullRef := registry.Reference{
Registry: r.Reference.Registry,
Repository: r.Reference.Repository,
Reference: reference,
}
ref = fullRef.String()
}
scope := r.Reference.Registry + "/" + r.Reference.Repository
imageRef := policy.ImageReference{
Transport: policy.TransportNameDocker,
Scope: scope,

Copilot uses AI. Check for mistakes.
reqs := e.policy.GetRequirementsForImage(image.Transport, image.Scope)

if len(reqs) == 0 {
// No requirements means reject by default for safety
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says “No requirements means reject by default for safety”, but the code returns a non-nil error when len(reqs) == 0. Either adjust the comment to match the behavior, or change the behavior to return (false, nil) if an empty requirement set is meant to be a clean deny decision.

Suggested change
// No requirements means reject by default for safety
// No requirements: treat as a policy error and reject by default for safety.

Copilot uses AI. Check for mistakes.
Comment on lines +388 to +411
// Create a temporary policy file in the home directory
homeDir, err := os.UserHomeDir()
if err != nil {
t.Skip("Cannot get home directory")
}

userPolicyDir := filepath.Join(homeDir, PolicyConfUserDir)
userPolicyPath := filepath.Join(userPolicyDir, PolicyConfFileName)

// Clean up any existing test policy
defer os.Remove(userPolicyPath)

// Create policy directory
if err := os.MkdirAll(userPolicyDir, 0755); err != nil {
t.Fatalf("failed to create policy directory: %v", err)
}

// Create a test policy
testPolicy := &Policy{
Default: PolicyRequirements{&InsecureAcceptAnything{}},
}
if err := testPolicy.Save(userPolicyPath); err != nil {
t.Fatalf("failed to save test policy: %v", err)
}
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestLoadDefault writes a real policy.json into the current user’s home directory ($HOME/.config/containers/policy.json) and removes it afterward. This makes the test suite stateful and can clobber a developer’s existing config (or behave differently depending on what already exists). Prefer isolating the home dir by setting HOME/USERPROFILE to t.TempDir() (via t.Setenv) and constructing paths under that temp home.

Copilot uses AI. Check for mistakes.
Comment on lines +87 to +102
// SetDefault sets the default policy requirements.
func (p *Policy) SetDefault(reqs ...PolicyRequirement) *Policy {
p.Default = reqs
return p
}

// SetTransportScope sets the policy requirements for a specific transport and scope.
func (p *Policy) SetTransportScope(transport TransportName, scope string, reqs ...PolicyRequirement) *Policy {
if p.Transports == nil {
p.Transports = make(map[TransportName]TransportScopes)
}
if p.Transports[transport] == nil {
p.Transports[transport] = make(TransportScopes)
}
p.Transports[transport][scope] = reqs
return p
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SetDefault assigns reqs (type []PolicyRequirement) directly to p.Default (type PolicyRequirements), and SetTransportScope assigns reqs directly into the TransportScopes map. These are distinct named slice types in Go, so this code will not compile. Convert the variadic slice to PolicyRequirements (and consider copying) before assigning.

Copilot uses AI. Check for mistakes.
shizhMSFT

This comment was marked as duplicate.

Copy link
Copy Markdown
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

This review was generated by GitHub Copilot using Claude Opus 4.6, mimicking the review style of shizhMSFT. This is not the real shizhMSFT — it is an AI-generated review persona. Please treat these comments as AI-assisted suggestions, not authoritative human feedback.

Good progress on addressing the previous comments — the package move to public and cross-platform support are welcome. However, there are several spec compliance issues that need attention before this can be merged. Always follow the spec since the spec is the standard.

Key concerns:

  1. Scope matching does not follow the spec. GetRequirementsForImage only performs exact match and transport-default fallback. Per containers-policy.json(5), for the docker: transport, scopes are matched as prefixes of the fully expanded form — a scope docker.io/library should match docker.io/library/nginx:latest. Wildcard subdomain matching (*.example.com) is also required. This is a fundamental correctness issue.

  2. signedBy field schema deviates from the spec. The spec says: "Exactly one of keyPath, keyPaths and keyData must be present." The implementation (a) accepts any combination instead of exactly one, and (b) adds a keyDatas field with a SignedByKeyData struct type that does not exist in the spec.

  3. sigstoreSigned is missing several spec-defined fields. The pki configuration, keyPaths, rekorPublicKeyPaths, and rekorPublicKeyDatas are all absent. If we choose not to implement them yet, this should be explicitly documented and the parser should reject unknown/unsupported fields rather than silently ignoring them.

  4. FulcioConfig.Validate() is incomplete. Per the spec, when fulcio is present, both oidcIssuer and subjectEmail are mandatory. The current validation only checks for CA path/data.

See inline comments for additional issues.


// GetRequirementsForImage returns the policy requirements for a given transport and scope.
// It follows the precedence rules: specific scope > transport default > global default.
func (p *Policy) GetRequirementsForImage(transport TransportName, scope string) PolicyRequirements {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the most critical issue in this PR. The spec states that for the docker: transport, "More general scopes are prefixes of individual-image scopes, and specify a repository (by omitting the tag or digest), a repository namespace, or a registry host". The current implementation only does exact string match.

For example, a policy with scope docker.io/library should match image docker.io/library/nginx:latest, but this code would fall through to the transport default instead. Wildcard subdomain matching (*.example.com) is also required by the spec.

Should we implement proper prefix-based scope matching with longest-prefix-wins semantics? This is fundamental to the correctness of the policy evaluation.

}

// Validate that at least one key source is provided
hasKey := r.KeyPath != "" || r.KeyData != "" || len(r.KeyPaths) > 0 || len(r.KeyDatas) > 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec says: "Exactly one of keyPath, keyPaths and keyData must be present." This validation only checks that at least one is present. It should reject configurations where multiple key sources are specified simultaneously.

// KeyPaths is a list of key paths (alternative to KeyPath)
KeyPaths []string `json:"keyPaths,omitempty"`
// KeyDatas is a list of inline key data (alternative to KeyData)
KeyDatas []SignedByKeyData `json:"keyDatas,omitempty"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signedBy spec does not define a keyDatas field nor a SignedByKeyData struct. The spec fields are: keyPath (string), keyPaths (array of strings), keyData (string, base64-encoded). Should we remove KeyDatas to match the spec?

}

// PRSigstoreSigned represents a sigstore signature policy requirement
type PRSigstoreSigned struct {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRSigstoreSigned is missing several fields defined in the spec:

  • keyPaths (array of key file paths)
  • pki (non-Fulcio X.509 certificate config with caRootsPath, caRootsData, caIntermediatesPath, caIntermediatesData, subjectHostname, subjectEmail)
  • rekorPublicKeyPaths (array of Rekor public key paths)
  • rekorPublicKeyDatas (array of base64-encoded Rekor public key data)

Also, the spec says keyDatas is ["base64-encoded-public-key-one-data", ...] — a simple string array, not []SigstoreKeyData. The current SigstoreKeyData struct with PublicKeyFile/PublicKeyData fields does not correspond to anything in the spec.

If we intentionally don't support some fields yet, we should document this and the JSON parser should reject policies containing unsupported fields rather than silently ignoring them.

Comment on lines +236 to +243
func (fc *FulcioConfig) Validate() error {
// At least CA path or data should be provided
if fc.CAPath == "" && len(fc.CAData) == 0 {
return fmt.Errorf("either caPath or caData must be specified")
}

return nil
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the spec: "If fulcio is present... Both oidcIssuer and subjectEmail are mandatory." The FulcioConfig.Validate() only checks for CA path/data. We need to validate OIDCIssuer and SubjectEmail as well.

Suggested change
func (fc *FulcioConfig) Validate() error {
// At least CA path or data should be provided
if fc.CAPath == "" && len(fc.CAData) == 0 {
return fmt.Errorf("either caPath or caData must be specified")
}
return nil
}
func (fc *FulcioConfig) Validate() error {
// At least CA path or data should be provided
if fc.CAPath == "" && len(fc.CAData) == 0 {
return fmt.Errorf("either caPath or caData must be specified")
}
// Per spec, both oidcIssuer and subjectEmail are mandatory when fulcio is used
if fc.OIDCIssuer == "" {
return fmt.Errorf("oidcIssuer is required for fulcio configuration")
}
if fc.SubjectEmail == "" {
return fmt.Errorf("subjectEmail is required for fulcio configuration")
}
return nil
}

}

// marshalWithType marshals a value and adds a "type" field to the resulting JSON
func marshalWithType(typeName string, v interface{}) ([]byte, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: marshalWithType does marshal → unmarshal-to-map → add type → marshal-again. This triple serialization is fragile — []byte fields will be double-base64-encoded if the intermediate map representation doesn't preserve the raw bytes correctly. Consider using a wrapper struct with an embedded type field instead.


imageRef := policy.ImageReference{
Transport: policy.TransportNameDocker,
Scope: r.Reference.Repository,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scope is set to r.Reference.Repository, but per the spec, docker scopes are the fully expanded form — e.g., docker.io/library/busybox:latest (not busybox:latest). Does r.Reference.Repository always produce the fully expanded form? If a user creates a Repository with reference library/nginx, will the scope be docker.io/library/nginx or just library/nginx? This matters for correct policy matching.


// checkPolicy validates the repository access against the configured policy.
// If no policy is configured (Policy is nil), this is a no-op.
func (r *Repository) checkPolicy(ctx context.Context, reference string) error {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Policy is checked for Fetch, Push, and Resolve, but not for Delete, Tag, or Manifests().PushReference(). Is this intentional? The spec-based policy.json is typically applied to pull/push operations, but we should document which operations are covered and which are not.

// NewEvaluator creates a new policy evaluator
func NewEvaluator(policy *Policy, opts ...EvaluatorOption) (*Evaluator, error) {
if policy == nil {
return nil, fmt.Errorf("policy cannot be nil")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: fmt.Errorf("policy cannot be nil") — should this be a sentinel error? In oras-go convention, consider returning an existing errdef error or at least using %w for wrappable errors.


const (
// PolicyConfUserDir is the user-level configuration directory for policy.json
PolicyConfUserDir = ".config/containers"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Go convention — exported constants that are implementation details of the package path resolution should be unexported unless SDK users need them. Do SDK users need PolicyConfUserDir, PolicyConfFileName, and PolicyConfSystemPath?

- Implement longest-prefix scope matching with wildcard subdomain
  support in GetRequirementsForImage per containers-policy.json spec
- Enforce exactly one key source in PRSignedBy (keyPath, keyPaths, or
  keyData) per spec
- Remove non-spec SignedByKeyData struct and KeyDatas field from
  PRSignedBy
- Change PRSigstoreSigned.KeyDatas to []string per spec
- Remove non-spec SigstoreKeyData struct
- Require OIDCIssuer and SubjectEmail in FulcioConfig.Validate per spec
- Validate that Policy.Default is non-empty per spec
- Use 0600/0700 file permissions in Save for security policy files
- Remove duplicate Load function, keep only LoadPolicy
- Unexport policy path constants (policyConfUserDir, etc.)
- Use errdef.ErrMissingReference sentinel for nil policy error

Signed-off-by: Terry Howe <thowe@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v3 Things belongs to version 3.x

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants